-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add geometry editor functions #554
Conversation
description: > | ||
Return true if the input geometry is a valid 2D geometry. | ||
|
||
For 3 dimensional and 4 dimensional geometries, the validity is still only tested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to have this restriction it'd be nice to either have 2d in the name or an option that specifies the number of dimensions to check against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I also noticed that even though postGIS sets this restriction (https://postgis.net/docs/ST_IsValid.html), some other backends don't specify the dimensionality. For example: https://docs.snowflake.com/en/sql-reference/functions/st_isvalid
@paleolimbot, do you have any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake only supports 2D, so they probably don't need to specify that.
As with all of these PRs, I would urge that Substrait copies PostGIS' (or simple features access) naming + functionality identically. Many of those decisions are a result of years of negotiation and there is a long history of database engineers coming in with a new hot take on how it should be done (e.g., the built-in Postgres geometry types that aren't part of PostGIS and SQLite's geo module support, neither of which are in common use).
That's a long way of saying I agree with what you have in this PR 🙂
- | ||
name: "flip_coordinates" | ||
description: > | ||
Return a version of the input geometry with the X and Y axis flipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be another 2D operation. We should probably state what happens to the other dimensions and/or rename the function to clarify its 2D nature.
description: > | ||
Return a version of the input geometry with duplicate consecutive points removed. | ||
|
||
If the `tolerance` argument is provided, consecutive points within the tolerance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a 2D point and a 3D point near the zero plane are nearby? Are those eligible to be considered duplicates or does the fact that they have different dimensionalities exclude them from being considered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems undocumented pretty much everywhere but it seems to be just 2D in the PostGIS implementation ( https://github.com/libgeos/geos/blob/main/src/operation/valid/RepeatedPointRemover.cpp#L50-L56 ). This function isn't in simple features access and was only added to a relatively new version of GEOS (so pretty low priority if it needs to be dropped).
extensions/functions_geometry.yaml
Outdated
- args: | ||
- value: geometry | ||
name: geom | ||
- value: fp32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is fp32 sufficient or do we want to support fp64 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to support fp64
816401c
to
3d16df0
Compare
3d16df0
to
a16f3a6
Compare
a16f3a6
to
451504a
Compare
@@ -21,16 +21,16 @@ scalar_functions: | |||
- | |||
name: "makeline" | |||
description: > | |||
Returns a linestring connecting the endpoint of geometry `x` to the begin point of | |||
geometry `y`. Repeated points at the beginning of input geometries are collapsed to a single point. | |||
Returns a linestring connecting the endpoint of geometry `geom1` to the begin point of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Joris Van den Bossche <[email protected]>
PR to add geometry editor functions